Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

resource/aws_iam_role: Use ListAttachedRolePoliciesPages #2857

Merged
merged 1 commit into from
Jan 18, 2018
Merged

resource/aws_iam_role: Use ListAttachedRolePoliciesPages #2857

merged 1 commit into from
Jan 18, 2018

Conversation

atsushi-ishibashi
Copy link
Contributor

@atsushi-ishibashi atsushi-ishibashi commented Jan 4, 2018 •
edited
Loading

Managed policies may exceed the first response of ListAttachedRolePolicies.

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 4, 2018
@bflad bflad added the service/iam Issues and PRs that pertain to the iam service. label Jan 11, 2018
@atsushi-ishibashi
Copy link
Contributor Author

@Ninir @bflad This is not enhanccement, but bug.

@bflad
Copy link
Contributor

bflad commented Jan 18, 2018

@atsushi-ishibashi are you hitting this and missing results? If so, do you have a lot of managed IAM roles or did AWS increase your role policy attachment limit? The IAM limits documentation states by default:

Managed policies attached to an IAM role: 10

I guess you could be hitting the second part of ListAttachedRolePoliciesInput documentation for MaxItems by default:

    // (Optional) Use this only when paginating results to indicate the maximum
    // number of items you want in the response. If additional items exist beyond
    // the maximum you specify, the IsTruncated response element is true.
    //
    // If you do not include this parameter, it defaults to 100. Note that IAM might
    // return fewer results, even when there are more results available. In that
    // case, the IsTruncated response element returns true and Marker contains a
    // value to include in the subsequent call that tells the service where to continue
    // from.
    MaxItems *int64 `min:"1" type:"integer"`

Please confirm what you're seeing and we'll prioritize accordingly. Thanks.

@atsushi-ishibashi
Copy link
Contributor Author

@bflad Thanks. Actually I didn't come across the bug but when I implemented #2388, I rememberd that I would fix pagination similar to ListRolePoliciesPages.

@bflad bflad added bug Addresses a defect in current functionality. and removed enhancement Requests to existing resources that expand the functionality or scope. labels Jan 18, 2018
@bflad bflad added this to the v1.7.1 milestone Jan 18, 2018
bflad
bflad approved these changes Jan 18, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and thanks for this fix @atsushi-ishibashi -- I'm going to pull in this PR right now for a few reasons:

  • Its small and I basically had already gone through it
  • We potentially were missing results since in the old code we were not checking NextToken and IAM was not guaranteeing all results in the first response according to the documentation
  • This brings the attached policy code in line with the inline policy code right below it
make testacc TEST=./aws TESTARGS='-run=TestAccAWSIAMRole'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSIAMRole -timeout 120m
=== RUN   TestAccAWSIAMRolePolicy_importBasic
--- PASS: TestAccAWSIAMRolePolicy_importBasic (9.65s)
=== RUN   TestAccAWSIAMRole_importBasic
--- PASS: TestAccAWSIAMRole_importBasic (8.39s)
=== RUN   TestAccAWSIAMRolePolicy_basic
--- PASS: TestAccAWSIAMRolePolicy_basic (14.30s)
=== RUN   TestAccAWSIAMRolePolicy_namePrefix
--- PASS: TestAccAWSIAMRolePolicy_namePrefix (7.08s)
=== RUN   TestAccAWSIAMRolePolicy_generatedName
--- PASS: TestAccAWSIAMRolePolicy_generatedName (7.15s)
=== RUN   TestAccAWSIAMRolePolicy_invalidJSON
--- PASS: TestAccAWSIAMRolePolicy_invalidJSON (1.17s)
=== RUN   TestAccAWSIAMRole_basic
--- PASS: TestAccAWSIAMRole_basic (8.78s)
=== RUN   TestAccAWSIAMRole_basicWithDescription
--- PASS: TestAccAWSIAMRole_basicWithDescription (17.71s)
=== RUN   TestAccAWSIAMRole_namePrefix
--- PASS: TestAccAWSIAMRole_namePrefix (8.55s)
=== RUN   TestAccAWSIAMRole_testNameChange
--- PASS: TestAccAWSIAMRole_testNameChange (14.10s)
=== RUN   TestAccAWSIAMRole_badJSON
--- PASS: TestAccAWSIAMRole_badJSON (0.80s)
=== RUN   TestAccAWSIAMRole_force_detach_policies
--- PASS: TestAccAWSIAMRole_force_detach_policies (9.82s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	107.549s

I'm going to quickly comb through the existing issues to see if this actually closes out any of them.

@bflad bflad merged commit a76cb53 into hashicorp:master Jan 18, 2018
@bflad bflad changed the title r/iam_role: Use ListAttachedRolePoliciesPages resource/aws_iam_role: Use ListAttachedRolePoliciesPages Jan 18, 2018
bflad added a commit that referenced this pull request Jan 18, 2018
@bflad
Update CHANGELOG for #2857
fe7bb68
@bflad
Copy link
Contributor

bflad commented Jan 22, 2018

This has been released in terraform-provider-aws version 1.7.1. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

drewsonne pushed a commit to drewsonne/terraform-provider-aws that referenced this pull request Mar 3, 2018
@bflad @drewsonne
Update CHANGELOG for hashicorp#2857
a1fda29
@ghost
Copy link

ghost commented Apr 8, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/iam Issues and PRs that pertain to the iam service.
Projects
None yet
Milestone
v1.7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@atsushi-ishibashi @bflad @Ninir